Bump about this PR (patch is rebased).

Its quite a while ago so iam not sure i remember it all 100% correct. The specification for the range coder the way it was written IIRC. Was to define how to decode it, leaving the details of the termination to the implementation. In practice this did not fully work out and we ended up with writing that 129 symbol that was then needed to be also considered on the decoder side to achieve correct read/write in all corner cases. But in fact this more strictly specified termination should allow some additional degree of bitstream error detection. So this is not really a bad thing. The missing 129 case could be a bug. The change in this pull request would imply that the 129 context symbol when read is always a 0. Are you sure that on all corner cases this is read back as a 0 ?

Are you sure that on all corner cases this is read back as a 0 ?

I wrote 0 here because the reference encoder puts 0 in all paths I saw (2 paths, as in the PR). As I don't understand the purpose of this 0 (I still don't get the reason the range coder needs to be "terminated" before starting to handle the next slice, when GOP size is more than 1, but it does not matter here), I guessed that 0 is needed.
But as the reference decoder (and any implementation I am aware about) discards the value, it could be decided that the value does not matter and in that case I change "zero" to "any" (would it mean that an encoder can put any value in it without problem with decoding?). In that case, we need to be clear that no decoder or conformance checker is authorized to consider 1 as an invalid value.

Beside the discussion about this "zero", is there any other issue preventing the patch to be merged?

What i meant is, are you sure that in every corner case where the encoder writes the termination 0 thingy, it is read as a 0. Because IIRC this was not true for the last symbol without this termination.

I don't understand well the maths nor the implementation behind the range coder, especially for the termination, I can only say that after quick checks, I did not find non 0 end while parsing the bitstream of a couple of files. Nothing exhaustive.

From what you say, I understand that it would be better to explicitly stipulate the reference encoder and decoder behaviors, something like:

The encoder SHOULD write a value of 0.
The decoder SHOULD ignore the value.

is it what you would expect?

If you are convinced that its always 0 then you could add a check with this to the decoder to detect errors.

I am convinced about nothing about the value, I just want to document the bitstream so people don't have to dig in FFmpeg code for correctly decoding a FFV1 slice, and also people would not create a bitstream compliant with spec but considered as buggy by FFmpeg developers.
I am just seeing that FFmpeg writes 0 during encoding and ignores the value during decoding, so I ask what is expected in a specification: exactly what is in the FFmpeg source code or is the bitstream more generic e.g. permitting to write 1 or is 0 always expected during decoding?

Actually, my question is the opposite of being convinced: before adding any check, I ask what is the expected bitstream specification, I suggest one specification from my understanding only for debate, as I did not create the corresponding code and I don't know what is exactly behind it (=I don't know why there is no check about this value in the decoder).
I also ask if you plan to add a check in the future: if you don't write now in spec that you'll write a test in FFmpeg code about this zero, you won't be able to do it without maybe breaking compatibility with files from another encoder compliant with the spec but not compliant with your updated decoder. I want to avoid that case by being explicit now about what is authorized or forbidden.

As there is a discussion about the value itself, I changed the patch with "zero" replaced by "terminate_range_coder" and meaning replaced by

The encoder SHOULD write a value of 0.
The decoder SHOULD ignore the value.

(= what is in FFmpeg for the moment, I prefer SHOULD over MUST because there is maybe a reason to inject something not 0 in the future, I did some tests and the decoder just ignores it, but I don't know if you want a MUST i.e. if you will add one day a test in FFmpeg for considering that 1 is a wrong value)

Still something blocking the merge?

If you are not convinced about anything, how do you want to document this ?
If you want to document it, as you say, you need to first understand it. Its a long time since i worked on this and its a rather abstract and disconnected part of the spec so i cannot just say with certanity that X or Y is guranteed true. I first would have to spend some time looking over all the stuff again
Also as you write "SHOULD", when should an implementation not follow this should ?
I think "SHOULD" should be used when there are cases where it makes sense to not do something and this is understood. But i have the feeling you use SHOULD because you are uncertain about the whole.
Also theres a 2nd issue in this. Termination isnt done after writing the 0. It was all sort of described in the range coder documentation but this was not complete i think. The addition of a "should write 0" in a seperate place for which it is not clear how this connects to the rest of the range coder documentation. I think this is not enough, some more information is needed for a reader to understand how to implement it

I'm uncertain of the status of this issue, but am currently in a discussion in cellar working group at https://etherpad.tools.ietf.org/p/notes-cellar-virtual04?useMonospaceFont=true. This issue seems like the only open issue that impacts our document from defining the v0, v1, v3 current state of FFV1. Can we resolve this and give FFV1 another review. I'd like to get a new version in the document tracker to support a working group last call.

something like this could work, but its not ideal as it would describe the encoder process which is not really want we want ...

pseudo-code (or any equivalent process)                       | type
--------------------------------------------------------------|-----
TerminateRangeCoder( version ) {                              |
    if (version == 1)                                         |
        zero                                                  | br
    range = 0xFF;                                             |
    low  += 0xFF;                                             |
    renorm_encoder();                                         |
    range = 0xFF;                                             |
    renorm_encoder();                                         |
}                                                             |

also ive posted some patches to the implementation, stuff spotted when looking over this

I prefer to have some explanation at the decoding level, unfortunately I still have difficulties to phrase the process, especially the difference between v3 and v0/v1.

And even with the proposed pseudo-code, "129" stuff in reference encoder/decoder seems still magical.

This is superseeded by #137 so i think theres no need to keep this open, could just add confusion. We could reopen this if someone feels this should stay open